Rate limit requests #267
Rate limit requests #267
Conversation
|
Warning Rate limit exceeded@sideninja has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 57 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates introduce rate-limiting capabilities to the API endpoints by integrating Changes
Sequence Diagram(s) (Beta)Silently ignored as it doesn't make sense for the given changes. Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum
Files selected for processing (10)
- api/api.go (24 hunks)
- api/errors/errors.go (1 hunks)
- api/pull.go (9 hunks)
- api/ratelimiter.go (1 hunks)
- api/server.go (1 hunks)
- api/stream.go (5 hunks)
- bootstrap/bootstrap.go (4 hunks)
- config/config.go (2 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/web3js/eth_rate_limit_test.js (1 hunks)
Files skipped from review due to trivial changes (2)
- api/errors/errors.go
- api/server.go
Additional Context Used
Biome (3)
tests/web3js/eth_rate_limit_test.js (3)
7-7: This let declares a variable that is only assigned once.
9-9: This let declares a variable that is only assigned once.
12-12: This let declares a variable that is only assigned once.
Additional comments not posted (10)
api/ratelimiter.go (1)
13-34: The implementation of the rate limiting function looks good.The function properly uses the limiter to control request rates and logs appropriately when the limit is reached. Good use of context and structured logging.
tests/web3js/eth_rate_limit_test.js (1)
5-45: The test logic for rate limiting is correctly implemented.The test effectively checks the rate limiting functionality by ensuring the number of successful and failed requests matches the expected values after hitting the rate limit. Good use of assertions to validate the behavior.
tests/e2e_web3js_test.go (1)
44-46: The addition of the rate-limit test case is appropriate.The new test case for rate-limiting is correctly added to the suite of end-to-end tests. This ensures that the rate-limiting functionality is tested in a real-world scenario.
bootstrap/bootstrap.go (1)
234-253: The initialization and integration of the rate limiter in the API setup are correctly implemented.The rate limiter is properly initialized with the configuration values and correctly passed to the API constructors (
blockchainAPI,streamAPI,pullAPI). This ensures that rate limiting is enforced across the API.config/config.go (1)
Line range hint
67-105: The updates to the configuration structure and parsing are correctly implemented.The addition of the
RateLimitfield to theConfigstruct and its parsing from the command-line flags are correctly done. This allows for easy configuration of rate limits through command-line parameters, enhancing the flexibility of the application.api/stream.go (1)
Line range hint
16-61: The integration of the rate limiter into theStreamAPIand its usage in thenewSubscriptionmethod are correctly implemented.The rate limiter is appropriately integrated into the
StreamAPIstruct and is effectively used in thenewSubscriptionmethod to enforce rate limits before creating new subscriptions. This ensures that the streaming API respects the configured rate limits, enhancing the robustness of the system.api/pull.go (2)
139-139: Ensure proper initialization ofratelimiterinPullAPI.The addition of the
ratelimiterfield to thePullAPIstruct aligns with the PR's objective to implement rate limiting. Ensure that theratelimiteris properly initialized and configured before being used in API methods.
Line range hint
148-157: Check error handling duringPullAPIinitialization.Ensure that all potential errors during the initialization of
PullAPIare handled appropriately, especially since theratelimiteris now part of the initialization parameters.api/api.go (2)
60-60: Properly integratelimiterinBlockChainAPI.The integration of the
limiterfield in theBlockChainAPIstruct is crucial for implementing rate limiting at the blockchain API level. Ensure that thelimiteris correctly configured and utilized in all relevant methods.
Line range hint
71-89: Validate error handling duringBlockChainAPIinitialization.Ensure that errors are properly handled during the initialization of
BlockChainAPI, especially with the addition of theratelimiterparameter.
| if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
Standardize error handling in rate limiting checks.
- if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil {
- return "", err
- }
+ if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil {
+ return rpc.ID(""), fmt.Errorf("rate limit exceeded: %w", err)
+ }Consider standardizing the error messages across all methods when the rate limit is exceeded. This will improve the consistency and clarity of error handling in the API.
Also applies to: 202-204, 248-249, 288-289, 340-341
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil { | |
| return "", err | |
| } | |
| if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil { | |
| return rpc.ID(""), fmt.Errorf("rate limit exceeded: %w", err) | |
| } |
| remote := core.MetadataFromContext(ctx).Remote | ||
| if remote == "NA" { | ||
| return nil // if no client identifier disable limit | ||
| } |
There was a problem hiding this comment.
Consider handling the case where remote is "NA" more securely.
Bypassing rate limiting based on a client identifier being "NA" could be a security risk if not handled properly elsewhere in the application. It's recommended to ensure that this condition is either necessary or handled with additional security checks.
api/api.go
Outdated
| if err := rateLimit(ctx, b.limiter, b.logger); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Refine error messages for rate limiting.
- if err := rateLimit(ctx, b.limiter, b.logger); err != nil {
- return nil, err
- }
+ if err := rateLimit(ctx, b.limiter, b.logger); err != nil {
+ return nil, fmt.Errorf("rate limit exceeded: %w", err)
+ }Enhance the clarity and consistency of error messages across all methods in BlockChainAPI when the rate limit is exceeded. This will help in better error tracking and user feedback.
Also applies to: 108-110, 127-129, 154-156, 173-175, 188-190, 210-212, 239-241, 268-270, 296-298, 320-322, 334-336, 363-365, 395-397, 434-436, 452-454, 481-483, 517-519, 574-576, 618-620, 650-652
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err := rateLimit(ctx, b.limiter, b.logger); err != nil { | |
| return nil, err | |
| } | |
| if err := rateLimit(ctx, b.limiter, b.logger); err != nil { | |
| return nil, fmt.Errorf("rate limit exceeded: %w", err) | |
| } |
|
|
||
| remote := core.MetadataFromContext(ctx).Remote | ||
| if remote == "NA" { | ||
| return nil // if no client identifier disable limit |
There was a problem hiding this comment.
is this controlable by the user (e.g. by manipulating headers)?
There was a problem hiding this comment.
It's assigned from the request, so it should be fine, but this actually made me think about the issue with proxy setup.
| @@ -1,6 +1,8 @@ | |||
| module github.com/onflow/flow-evm-gateway | |||
|
|
|||
| go 1.20 | |||
| go 1.22 | |||
There was a problem hiding this comment.
is this necessary? it's a good practice to do this sort of upgrade separately from a feature change in case you have to rollback.
There was a problem hiding this comment.
wasn't aware this was done, will revert
|
@sjonpaulbrown how do we have envoy setup to forward the origin IP address? I can see this documentation https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_features/ip_transparency |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/helpers.go (1 hunks)
Additional comments not posted (1)
tests/helpers.go (1)
148-148: Verify the hardcodedRateLimitvalue.
Envoy is configured with |
# Conflicts: # api/api.go
| return nil, err | ||
| } | ||
|
|
||
| return (*hexutil.Big)(b.config.EVMNetworkID), nil |
There was a problem hiding this comment.
Does it make sense to rate limit a simple endpoint such as this? It only returns a value which is stored in a config object, does not make any DB interactions.
And I assume it is a frequently called endpoint, needed for transaction signing etc.
api/api.go
Outdated
| if err := rateLimit(ctx, b.limiter, b.logger); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return (*hexutil.Big)(b.config.GasPrice), nil |
There was a problem hiding this comment.
Same here, that's a fairly cheap and simple endpoint. No DB interactions.
There was a problem hiding this comment.
I can remove on the simple ones.
api/api.go
Outdated
| if err := rateLimit(ctx, b.limiter, b.logger); err != nil { | ||
| return common.Address{}, err | ||
| } | ||
|
|
||
| return b.config.Coinbase, nil |
There was a problem hiding this comment.
Same here, that's a fairly cheap and simple endpoint. No DB interactions.
go.mod
Outdated
| go 1.20 | ||
| go 1.22 | ||
|
|
||
| toolchain go1.22.3 |
There was a problem hiding this comment.
I am not sure how this one gets in 😅
| @@ -1,6 +1,8 @@ | |||
| module github.com/onflow/flow-evm-gateway/integration | |||
|
|
|||
| go 1.20 | |||
| go 1.22 | |||
There was a problem hiding this comment.
We probably don't want these changes to go in. Including the line:
toolchain go1.22.3There was a problem hiding this comment.
The rate limiter library I use requires v1.22.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- api/api.go (24 hunks)
- api/server.go (1 hunks)
- bootstrap/bootstrap.go (4 hunks)
- tests/helpers.go (2 hunks)
- tests/web3js/eth_rate_limit_test.js (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- api/server.go
- bootstrap/bootstrap.go
- tests/helpers.go
Additional context used
Biome
tests/web3js/eth_rate_limit_test.js
[error] 7-7: This let declares a variable that is only assigned once.
[error] 13-13: This let declares a variable that is only assigned once.
[error] 16-16: This let declares a variable that is only assigned once.
Additional comments not posted (4)
api/api.go (4)
Line range hint
71-89: The initialization ofBlockChainAPIwith rate limiting is correctly implemented.
98-103: Rate limiting implementation inChainIdfunction is correct and efficient.
126-130: Proper implementation of rate limiting in theSyncingfunction.
154-157: Correct application of rate limiting inSendRawTransaction.
| it('rate limit after X requests', async function () { | ||
| this.timeout(0) | ||
| setTimeout(() => process.exit(0), 5000) // make sure the process exits | ||
| let ws = new Web3("ws://127.0.0.1:8545") |
There was a problem hiding this comment.
Use const for variables that are only assigned once.
- let ws = new Web3("ws://127.0.0.1:8545")
+ const ws = new Web3("ws://127.0.0.1:8545")
- let requestLimit = 50
+ const requestLimit = 50
- let requests = 60
+ const requests = 60This change ensures that these variables, which do not change after their initial assignment, are declared as const for better code clarity and to prevent accidental reassignments.
Also applies to: 13-13, 16-16
Committable suggestion was skipped due low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- api/api.go (23 hunks)
Additional comments not posted (5)
api/api.go (5)
18-18: Added import forgithub.jparrowsec.cn/sethvargo/go-limiter.This import is necessary for the rate limiting functionality being added across various methods in this file.
60-60: Addedlimiterfield toBlockChainAPIstruct.This is essential for storing the rate limiter instance that will be used across various API methods to enforce rate limits.
Line range hint
71-89: ModifiedNewBlockChainAPIto accept alimiter.Storeand return an error.This change is crucial for initializing the
BlockChainAPIwith a rate limiter. The error return is appropriate to handle potential issues during initialization.
98-100: TheChainIdmethod now accepts acontext.Contextand returns an error.This modification aligns with the need to incorporate rate limiting and error handling in the method.
114-116: Rate limiting applied to various methods.The consistent application of rate limiting across these methods ensures that the API can handle high traffic gracefully by limiting the number of requests per second.
Also applies to: 133-135, 160-162, 185-187, 207-209, 236-238, 265-267, 293-295, 322-324, 351-353, 383-385, 422-424, 440-442, 469-471, 505-507, 562-564, 606-608, 638-640
…gor/rate-limit-work
Closes: #265
Description
Rate limit the requests on both HTTP and WebSockets. Unfortunately, this couldn't be a simple HTTP middleware since this wouldn't capture WS requests, if we wanted to make it handle all requests over WS we would have to have access to the connection created, but unfortunately, that is not exposed on the go-Ethereum server implementation so we have to explicitly call it on all the handlers.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests